Skip to content

net: fix net.Server keepalive and noDelay #43497

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 22, 2022
Merged

net: fix net.Server keepalive and noDelay #43497

merged 1 commit into from
Jun 22, 2022

Conversation

@theanarkh
Copy link
Contributor

@theanarkh theanarkh commented Jun 19, 2022 •
edited by VoltrexKeyva
Loading

The setXXX function should be called on clientHandle instead of server handle.

The code below can trigger the bug.

server.js

const net = require('net');
net.createServer({
    keepAlive: true,
    keepAliveInitialDelay: 1000
}, () => {
    
}).listen(8080);

client.js

const net = require('net');
net.createConnection({
    port:8080,
});

the code above do not send keepalive packet after 1s.

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Affected subsystem: net

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem. labels Jun 19, 2022
@mscdex
Copy link
Contributor

mscdex commented Jun 19, 2022

This needs a test.

@theanarkh theanarkh force-pushed the fix_net_keepalive branch from fe4c51d to 6651127 Compare June 19, 2022 18:26
@theanarkh
Copy link
Contributor Author

This needs a test.

Thanks, done !

@theanarkh theanarkh force-pushed the fix_net_keepalive branch from 6651127 to 18ed102 Compare June 19, 2022 18:31
violethaze74
violethaze74 reviewed Jun 19, 2022
View reviewed changes
Copy link

@violethaze74 violethaze74 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

theanarkh:fix_net_keepalive

ShogunPanda
ShogunPanda approved these changes Jun 19, 2022
View reviewed changes
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Thanks for noticing it!

@theanarkh
Copy link
Contributor Author

@mcollina Hi. Can you help review this PR ? Thanks!

mcollina
mcollina approved these changes Jun 22, 2022
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Jun 22, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 22, 2022
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/44794/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/44798/

@mcollina mcollina added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jun 22, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 22, 2022
@nodejs-github-bot nodejs-github-bot merged commit dbe5874 into nodejs:main Jun 22, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in dbe5874

This was referenced Jun 23, 2022
Open
Open
F3n67u pushed a commit to F3n67u/node that referenced this pull request Jun 24, 2022
@theanarkh @F3n67u
net: fix net.Server keepalive and noDelay
f1cd06c 
PR-URL: nodejs#43497
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
This was referenced Jun 25, 2022
Open
Open
Open
targos pushed a commit that referenced this pull request Jul 12, 2022
@theanarkh @targos
net: fix net.Server keepalive and noDelay
47a2522 
PR-URL: #43497
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@targos targos mentioned this pull request Jul 12, 2022
Merged
targos pushed a commit that referenced this pull request Jul 20, 2022
@theanarkh @targos
net: fix net.Server keepalive and noDelay
63d05b5 
PR-URL: #43497
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
@theanarkh @targos
net: fix net.Server keepalive and noDelay
e4a5bd1 
PR-URL: #43497
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@targos targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@theanarkh @guangwong
net: fix net.Server keepalive and noDelay
4e373b2 
PR-URL: nodejs/node#43497
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@ShogunPanda ShogunPanda ShogunPanda approved these changes

+1 more reviewer

@violethaze74 violethaze74 violethaze74 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. net Issues and PRs related to the net subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@theanarkh @nodejs-github-bot @mscdex @mcollina @ShogunPanda @violethaze74